Skip to content

Fix/small forecasting pipeline fixes#2011

Merged
Flix6x merged 37 commits intomainfrom
fix/small-forecasting-pipeline-fixes
Mar 18, 2026
Merged

Fix/small forecasting pipeline fixes#2011
Flix6x merged 37 commits intomainfrom
fix/small-forecasting-pipeline-fixes

Conversation

@BelhsanHmida
Copy link
Contributor

@BelhsanHmida BelhsanHmida commented Mar 9, 2026

Description

This PR includes several small fixes and improvements to the forecasting pipeline jobs:

  • Fix the TypeError: Job.__init__() missing 1 required argument: 'connection' raised by the wrap-up job.
  • Improve the returned job information by including the number of jobs created together with the wrap-up job ID.
  • Update tests to reflect the updated job return structure.
  • Add db session commit before the forecasting jobs are created to prevent data_source not in database error
  • add n_cycles in calculation of forecasts assertion in test_forecasting_pipeline.py to correctly test how many forecasts generated in case of multiple cycles

These changes ensure that forecasting jobs run correctly and that job metadata returned by the pipeline is clearer and easier to interpret.


Sign-off

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
@read-the-docs-community
Copy link

read-the-docs-community bot commented Mar 9, 2026

Documentation build overview

📚 flexmeasures | 🛠️ Build #31857658 | 📁 Comparing 39351a3 against latest (f1707fc)


🔍 Preview build

Show files changed (4 files in total): 📝 4 modified | ➕ 0 added | ➖ 0 deleted
File Status
changelog.html 📝 modified
genindex.html 📝 modified
_autosummary/flexmeasures.data.models.forecasting.pipelines.train_predict.html 📝 modified
api/v3_0.html 📝 modified

@nhoening nhoening added this to the 0.31.2 milestone Mar 9, 2026
Flix6x added 7 commits March 9, 2026 13:22
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
…dated

Signed-off-by: F.N. Claessen <claessen@seita.nl>
…ters

Signed-off-by: F.N. Claessen <claessen@seita.nl>
…ycle job

Signed-off-by: F.N. Claessen <claessen@seita.nl>
@Flix6x
Copy link
Contributor

Flix6x commented Mar 9, 2026

Thanks. I also pushed some changes to the test myself now, which now covers the case where more than 1 cycle is asked, thereby resulting in a wrap-up job being returned. We did not cover that yet. There was also a mistake in an if-statement in the test that resulted in us checking each item in an empty list of cycle_job_ids. See f26c41b.

The new test case unexpectedly failed on creating 3 forecasts instead of the expected 2 (one per cycle). Could you please investigate why? From inspecting the forecasts, I believe the last one doesn't make sense. Also the assert comment needs to be updated. None of the test cases lead to 4 forecasts per horizon, I think.

        assert (
            len(forecasts) == m_viewpoints * n_hourly_horizons * n_events_per_horizon
        ), f"we expect 4 forecasts per horizon for each viewpoint within the prediction window, and {m_viewpoints} viewpoints with each {n_hourly_horizons} hourly horizons"

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
… pipeline

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…ecasts

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…rity

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
@BelhsanHmida
Copy link
Contributor Author

Thanks. I also pushed some changes to the test myself now, which now covers the case where more than 1 cycle is asked, thereby resulting in a wrap-up job being returned. We did not cover that yet. There was also a mistake in an if-statement in the test that resulted in us checking each item in an empty list of cycle_job_ids. See f26c41b.

The new test case unexpectedly failed on creating 3 forecasts instead of the expected 2 (one per cycle). Could you please investigate why? From inspecting the forecasts, I believe the last one doesn't make sense. Also the assert comment needs to be updated. None of the test cases lead to 4 forecasts per horizon, I think.

I investigated this.

The extra forecast comes from viewpoints within each cycle. In this case:

  • start = 2025-01-08 00:00+02:00
  • end = 2025-01-09 00:00+02:00
  • retrain-frequency = 12h
  • forecast-frequency = 12h
  • max-forecast-horizon = 1h
    So the pipeline runs 2 cycles, but each cycle also generates 2 viewpoints because the predict window is still 24h and the predict pipeline uses predict_period_in_hours / forecast_frequency = 24 / 12 = 2 viewpoints per cycle.

That gives these forecasted events:

  • cycle 1, viewpoint 1: event at 2025-01-08 00:00+02:00
  • cycle 1, viewpoint 2: event at 2025-01-08 12:00+02:00
  • cycle 2, viewpoint 1: event at 2025-01-08 12:00+02:00
  • cycle 2, viewpoint 2: event at 2025-01-09 00:00+02:00
    So the “last” forecast does make sense: it is the second viewpoint of the second cycle. The overlap is at 2025-01-08 12:00+02:00, which is forecast once from the first cycle and then again from the second cycle with a newer belief time.

This is also why:

with most_recent_beliefs_only=True, you see 3 rows, because the two beliefs for 2025-01-08 12:00+02:00 collapse to the most recent one
with most_recent_beliefs_only=False, you see the full 4 saved forecasts

to more correctly test which forecasts are being saved into the sensor i set most_recent_beliefs_only=False in sensor.search_beliefs and introduced number of cycles in calculation of forecasts assertion

@Flix6x
Copy link
Contributor

Flix6x commented Mar 11, 2026

Thank you for the clear explanation. That almost clears it up for me completely. Two lingering doubts:

Why does the second cycle have a viewpoint exactly at the end? That feels odd to me. The only case where that would actually make sense to me is when forecasting an instantaneous sensor (for which the last event in the predict period is exactly at the end).

And, if the end is meant to be included, why does the first cycle not include the end? That looks inconsistent, doesn't it?

@BelhsanHmida
Copy link
Contributor Author

BelhsanHmida commented Mar 11, 2026

Thank you for the clear explanation. That almost clears it up for me completely. Two lingering doubts:

Why does the second cycle have a viewpoint exactly at the end? That feels odd to me. The only case where that would actually make sense to me is when forecasting an instantaneous sensor (for which the last event in the predict period is exactly at the end).

And, if the end is meant to be included, why does the first cycle not include the end? That looks inconsistent, doesn't it?

yes true the last viewpoint we should see is end - sensor_resolution. i'm looking into this

@BelhsanHmida
Copy link
Contributor Author

BelhsanHmida commented Mar 17, 2026

Thank you for the clear explanation. That almost clears it up for me completely. Two lingering doubts:
Why does the second cycle have a viewpoint exactly at the end? That feels odd to me. The only case where that would actually make sense to me is when forecasting an instantaneous sensor (for which the last event in the predict period is exactly at the end).
And, if the end is meant to be included, why does the first cycle not include the end? That looks inconsistent, doesn't it?

yes true the last viewpoint we should see is end - sensor_resolution. i'm looking into this

i identified the issue to be in base_pipeline when we assigned the end date when splitting the data-frames. i think this issue should be handled in a separate pr.

@Flix6x
Copy link
Contributor

Flix6x commented Mar 17, 2026

Okay, please create the issue for it.

Flix6x added 12 commits March 17, 2026 15:57
…n.commit() some time to finish writing a new source to the db

Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
This reverts commit 888b980.

Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
… we put it back into the session we are committing

Signed-off-by: F.N. Claessen <claessen@seita.nl>
…b.session.commit() some time to finish writing a new source to the db"

This reverts commit 4ebaa97.

Signed-off-by: F.N. Claessen <claessen@seita.nl>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Flix6x and others added 3 commits March 18, 2026 09:26
Signed-off-by: F.N. Claessen <claessen@seita.nl>
- Fix 'viewpoint' -> 'viewpoints' in comment
- Add ', and' before n_cycles in assertion message for clarity
- Improve comment on db.session.merge() to explain why merge is needed
  before commit (get_or_create_source flushes but does not commit)
… method; cherry-picked from cbff3b4

Co-authored-by: Flix6x <30658763+Flix6x@users.noreply.github.com>
@Flix6x Flix6x added the bug Something isn't working label Mar 18, 2026
@Flix6x Flix6x merged commit f3c22f3 into main Mar 18, 2026
11 checks passed
@Flix6x Flix6x deleted the fix/small-forecasting-pipeline-fixes branch March 18, 2026 08:59
Flix6x pushed a commit that referenced this pull request Mar 18, 2026
* fix: enhance job ID return structure in TrainPredictPipeline

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* fix: update forecasting job return structure in SensorAPI

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* fix: update job fetching logic in test_train_predict_pipeline

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* chore: remove commented-out breakpoint in test_forecasting.py

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* fix: as_job is no longer in  parameters

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* fix: update job count retrieval in add_forecast function

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* fix: add connection queue to fetch job

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* style: black

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* docs: changelog entry

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* feat: check if wrap-up job actually finished rather than failed

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* feat: add test case for 2 cycles, yielding 2 jobs and a wrap-up job

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* dev: comment out failing assert, which needs to be investgated and updated

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* refactor: move checking the status of the wrap-up job to where it matters

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* fix: use job ID itself in case the returned job is the one existing cycle job

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* fix: add db.commit before forecasting jobs are created

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* dev: uncomment test assertion statement

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* Test(feat): search all beliefs forecasts saved into the sensor by the pipeline

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* test(feat): add n_cycles variable to use to account for length of forecasts

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* style: run  pre-commit

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* fix: improve assertion message in test_train_predict_pipeline for clarity

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>

* fix: first create all jobs, then queue all jobs, giving the db.session.commit() some time to finish writing a new source to the db

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* feat: enqueue job only after the transactional request

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* Revert "feat: enqueue job only after the transactional request"

This reverts commit 888b980.

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* docs: resolve silent merge conflict in changelog

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* docs: delete duplicate changelog entry

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* docs: add release date for v0.31.2

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* docs: advance a different bugfix to v0.31.2

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* fix: self.data_source found itself in a different session somehow, so we put it back into the session we are committing

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* Revert "fix: first create all jobs, then queue all jobs, giving the db.session.commit() some time to finish writing a new source to the db"

This reverts commit 4ebaa97.

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* refactor: move asserts to where they matter

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* fix: cherry-pick from d40246f

Signed-off-by: F.N. Claessen <claessen@seita.nl>

* fix: improve comments and assertions from code review

- Fix 'viewpoint' -> 'viewpoints' in comment
- Add ', and' before n_cycles in assertion message for clarity
- Improve comment on db.session.merge() to explain why merge is needed
  before commit (get_or_create_source flushes but does not commit)

* refactor: move reattachment of sensor objects to session into a class method; cherry-picked from cbff3b4

Co-authored-by: Flix6x <30658763+Flix6x@users.noreply.github.com>

---------

Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Co-authored-by: F.N. Claessen <claessen@seita.nl>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Flix6x <30658763+Flix6x@users.noreply.github.com>

(cherry picked from commit f3c22f3)
Signed-off-by: F.N. Claessen <claessen@seita.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Forecasting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants